Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FIX: Bring pk_ops.h back as public header #3879

Merged
merged 3 commits into from
Feb 16, 2024
Merged

FIX: Bring pk_ops.h back as public header #3879

merged 3 commits into from
Feb 16, 2024

Conversation

reneme
Copy link
Collaborator

@reneme reneme commented Jan 9, 2024

See #3878 for details.

This also adds a new example, with three objectives:

  • Showcase how to integrate custom public-key algorithms
  • Explain how one could use Botan to build a hybrid public-key scheme (e.g. in the transition to PQC)
  • Act as a regression test for the ability of applications to build custom public-key schemes using Botan's API

@reneme reneme mentioned this pull request Jan 9, 2024
Closed
@reneme reneme self-assigned this Jan 9, 2024
@reneme reneme added this to the Botan 3.3.0 milestone Jan 9, 2024
@reneme reneme added the bug label Jan 9, 2024
@reneme reneme force-pushed the fix/custom_pubkey branch 2 times, most recently from d6676ec to ad34def Compare January 9, 2024 12:16
@coveralls
Copy link

coveralls commented Jan 9, 2024 •
edited
Loading

Coverage Status

coverage: 92.004% (-0.001%) from 92.005%
when pulling afc7bf7 on Rohde-Schwarz:fix/custom_pubkey
into 1e77255 on randombit:master.

@randombit
Copy link
Owner

Do we really have to make pk_ops_impl.h public here? That was always, starting from the commit that added it, an internal header, and tbh I'd rather not expose it since then it becomes a public API that we have to maintain compatability with forever. As I'm sure you've noticed, it's a lot easier to perform refactorings when it's provably the case that no external application is using that interface.

And while pk_ops.h being internal does indeed prevent any application from implementing another key type, pk_ops_impl.h are effectively just helpers; an application can do a KDF using the usual library APIs. This is slightly less convenient for the user, but also this is a really niche situation.

@reneme
Copy link
Collaborator Author

reneme commented Jan 9, 2024

Yeah, user convenience was the only reasoning. And I actually had my doubts, because the interplay between the base-classes in pk_ops.h with the helpers isn't really obvious. If you're saying that it wasn't public historically either, let's just hide it again.

I can't work on that today anymore, though. Will rework this first thing tomorrow.

@reneme reneme changed the title FIX: Bring pk_ops.h and pk_ops_impl.h back as public headers FIX: Bring pk_ops.h back as public header Jan 10, 2024
@reneme
Copy link
Collaborator Author

reneme commented Jan 10, 2024

This now brings back pk_ops.h as a public header, only. The new example had to be adapted accordingly.

Also, I previously forgot to annotate the classes in pk_ops.h as BOTAN_PUBLIC_API(3, 3). And I added a note in the migration guide about the accidental removal between 3.0 and 3.2.

@reneme reneme force-pushed the fix/custom_pubkey branch 2 times, most recently from ab9745d to 5aef1ff Compare January 10, 2024 10:27
randombit
randombit requested changes Jan 10, 2024
View reviewed changes
src/lib/pubkey/pk_ops.h Outdated
@@ -33,7 +38,7 @@ namespace PK_Ops {
/**
* Public key encryption interface
*/
class Encryption {
class BOTAN_PUBLIC_API(3, 3) Encryption {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be BOTAN_UNSTABLE_API.

Using this interface requires deriving, which is not covered by our current SemVer guarantee (doc/sem_ver.rst) and already I know we have to change at least the decryption API to support some side channel mitigations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@reneme
Make pk_ops.h public
b38e50a 
It used to be public in Botan 2.x but was removed from the public
interface in Botan 3.x. Though, this header is needed by applications
that wish to implement custom public-key algorithms.

Closes randombit#3878
@reneme
Document the erroneous removal of pk_ops.h
1f301aa
@reneme
Add example for hybrid key encapsulation
afc7bf7 
This also acts as a regression test for the ability of
applications to implement custom public-key algorithms.
@reneme reneme requested a review from randombit January 22, 2024 07:16
randombit
randombit approved these changes Feb 15, 2024
View reviewed changes
Copy link
Owner

@randombit randombit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@reneme reneme merged commit 3dc59fc into randombit:master Feb 16, 2024
39 checks passed
@reneme reneme deleted the fix/custom_pubkey branch February 16, 2024 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@securitykernel securitykernel securitykernel approved these changes

@randombit randombit randombit approved these changes

Assignees

@reneme reneme

Labels
bug
Projects
None yet
Milestone
Botan 3.3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@reneme @coveralls @randombit @securitykernel